portfwd: drain the buffer before leaving on EOF - #1228
Merged
Conversation
ejohnstown
requested review from
wolfSSL-Fenrir-bot
and
a lite review from Copilot
September 3, 2026 00:18
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new EOF handling currently treats recv() < 0 as EOF (masking socket errors), and the new shell test uses [ with -a in loop conditions in a way that is less portable/error-prone than the repo’s existing style.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes a port forwarding edge case where portfwd_worker() would exit immediately on local socket EOF, potentially dropping buffered data that had not yet been accepted by wolfSSH_ChannelSend() (e.g., while SSH window credits are in flight). It also adds a new bulk forward test to exercise transfers spanning multiple SSH windows.
Changes:
- Update
examples/portfwd/portfwd.cto stop polling the local socket on EOF and keep looping until the buffered outbound data is drained. - Add
scripts/fwd-bulk.testto push a multi-window payload through a local direct-tcpip forward and validate byte-for-byte integrity. - Register the new test script in
scripts/include.amso it runs undermake check.
File summaries
| File | Description |
|---|---|
| scripts/include.am | Adds the new bulk forwarding test script to the check scripts list. |
| scripts/fwd-bulk.test | New bulk data forwarding test that verifies multi-window payload forwarding correctness. |
| examples/portfwd/portfwd.c | Adjusts EOF handling to drain the local-to-SSH buffer before exiting the worker loop. |
Review details
Suppressed comments (2)
scripts/fwd-bulk.test:172
- Using
[with-ainside a single test expression is error-prone and less portable across /bin/sh implementations. Prefer&&with separate test brackets.
while [ "$got" -lt "$payload_size" -a "$counter" -lt "$transfer_limit" ]; do
scripts/fwd-bulk.test:159
- Using
[with-ainside a single test expression is error-prone and less portable due to historical precedence/parse quirks across /bin/sh implementations. Prefer combining two test brackets with&&instead.
while [ ! -s "$fwd_ready_file" -a "$counter" -lt 20 ]; do
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A zero read on the local socket left the loop at once, and anything read but not yet accepted by wolfSSH_ChannelSend() went with it. With the peer's window full that tail is up to a buffer's worth, so a transfer that ends while the window is being credited comes out short. - stop polling the socket on end-of-input and keep looping until the buffer is empty - leave anyway once the channel is gone, so a forward whose open failed cannot hold the loop on a buffer nothing can take
scripts/fwd.test sends eight short lines, so nothing in the suite drives a forwarding channel past its first window, and a forward that stalled once the window needed crediting would go unnoticed. - push a payload several windows long through a local direct-tcpip forward and compare the bytes that arrive - end a second transfer one window plus a short tail in, where the tail is what portfwd still holds at end-of-input, and repeat it since the window credit beating that tail is a race - give the listening nc its stdin from a fifo a sleep holds open: reading end-of-input makes nc close the connection, which truncates the transfer and looks exactly like a stall - bail out early once the byte count stops moving, so a real stall reports in seconds - dump the logs on failure, check the listening nc came up, and take ports clear of the ones fwd.test.expect hardcodes, so a squatted or shared port is not reported as a stall
ejohnstown
force-pushed
the
ccb-phase2-2
branch
from
September 3, 2026 21:21
ec3d5e9 to
1fa0905
Compare
wolfSSL-Fenrir-bot
previously requested changes
Sep 3, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1228
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
philljj
approved these changes
Sep 4, 2026
philljj
dismissed
wolfSSL-Fenrir-bot’s stale review
September 4, 2026 01:40
to be addressed in separate PR
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A zero read on the local socket left portfwd_worker()'s loop at once and
took with it anything wolfSSH_ChannelSend() had not yet accepted, so a
transfer ending while the window is being credited came out short.
buffer is empty.
cannot hold the loop on a buffer nothing can take.
forward, since scripts/fwd.test never drives past the first window.
which is the shape that loses data without the drain.